Documentation update for Ability to resolve with "nothing" #396
Conversation
| ### Improvements | ||
|
|
||
| The AsyncNodePluginPlugin class is now capable of handling the below : | ||
| - The resolvedMapping map now sets the node.id as the key and either parsedNode or node as the value, depending on whether parsedNode is truthy. | ||
| - This means that the plugin is now capable of resolving the async node even when the consumer returns a null/undefined value. |
There was a problem hiding this comment.
Rather than document the implementation details, can we document what it enables? Maybe something like:
| ### Improvements | |
| The AsyncNodePluginPlugin class is now capable of handling the below : | |
| - The resolvedMapping map now sets the node.id as the key and either parsedNode or node as the value, depending on whether parsedNode is truthy. | |
| - This means that the plugin is now capable of resolving the async node even when the consumer returns a null/undefined value. | |
| ### Edge cases | |
| If for some reason, the process for resolving some `AsyncNode` fails, or is no longer relevant, you can resolve the promise for that `AsyncNode` with `undefined` or `null` to replace that node with "nothing"... |
There was a problem hiding this comment.
If you want to document the implementation changes, you should put those in the release notes for this PR
There was a problem hiding this comment.
but we are actually returning the existing node itself instead of nothing
There was a problem hiding this comment.
Example for adding release notes:
#296 (comment)
There was a problem hiding this comment.
should we be more intentional about the use case, rather than stating this to be a fallback option of sort
There was a problem hiding this comment.
is this a question to me or @sugarmanz ?
There was a problem hiding this comment.
both, right now it's framed as a fallback option, I'm thinking we make this more intentional because i'm thinking about the future node removal work
There was a problem hiding this comment.
I think that's covered with:
or is no longer relevant
But we could have an explicit use case around using async node to power toasts, or something like that.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## bazel-6 #396 +/- ##
========================================
Coverage 91.82% 91.82%
========================================
Files 339 339
Lines 27007 27007
Branches 1956 1956
========================================
Hits 24799 24799
Misses 2195 2195
Partials 13 13 ☔ View full report in Codecov by Sentry. |
|
Pushing this to backlog as a collective decision has been made that this will be done after all the related iterations are in , so closing this as of now |
Documentation update for Ability to resolve with "nothing"
Change Type (required)
Indicate the type of change your pull request is:
patchminormajorDoes your PR have any documentation updates?